Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let tctl make plugins #41038

Merged
merged 22 commits into from
May 17, 2024
Merged

Let tctl make plugins #41038

merged 22 commits into from
May 17, 2024

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Apr 30, 2024

Allows tctl to install an Okta integration without having the user resort to crafting a PluginRequest in JSON. It's not as automated as the Web-based installer, but is a reasonable mid-point if you need more control over integration behaviour.

Example:

$ tctl plugins install okta --org https://example.com --owner "admin" -g "group #1" --api-token ${API-TOKEN}

Note: Currently defaults to --sync--users and --sync-groups being enabled for parity with the Web-based installer, but I'm not wedded to it. Perhaps defaulting to "off" would be less surprising for a CLI.

Full option set:

❯ ./e/build/tctl plugins install okta --help
usage: tctl plugins install okta --org=ORG --api-token=API-TOKEN [<flags>]

install an okta integration

Flags:
  -d, --[no-]debug        Enable verbose logging to stderr
  -c, --config            Path to a configuration file [/etc/teleport.yaml]. Can also be set via the TELEPORT_CONFIG_FILE environment variable.
      --auth-server       Attempts to connect to specific auth/proxy address(es) instead of local auth [127.0.0.1:3025]
  -i, --identity          Path to an identity file. Must be provided to make remote connections to auth. An identity file can be exported with 'tctl auth sign'
      --[no-]insecure     When specifying a proxy address in --auth-server, do not verify its TLS certificate. Danger: any data you send can be intercepted or modified by an attacker.
      --name              name of the plugin resource to create
      --org               URL of Okta organization
      --api-token         API token to install with plugin
      --saml-connector    SAML connector to link to plugin
      --app-id            Okta ID of the APP used for SSO via SAML
      --scim-token        SCIM token to install with plugin
      --[no-]sync-users   Enable user synchronization
  -o, --owner             Set default owners for synced AccessLists
      --[no-]sync-groups  Enable group to AccessList synchronization
  -g, --group             Add a group filter. Supports globbing by default. Enclose in `^pattern$` for full regex support.
  -a, --app               Add an app filter. Supports globbing by default. Enclose in `^pattern$` for full regex support.

Also moves some constants (e.g. Okta plugin label names) from Enterprise to OSS where they can be seen from tctl. A following PR will update the enterprse definitions to point back to the new constants created here.

Changelog: Added support for creating Okta integrations in tctl.

@tcsc tcsc requested review from smallinsky and r0mant May 1, 2024 12:40
@tcsc tcsc marked this pull request as ready for review May 1, 2024 12:40
@github-actions github-actions bot added size/md tctl tctl - Teleport admin tool labels May 1, 2024
@github-actions github-actions bot requested review from jakule and marcoandredinis May 1, 2024 12:41
tool/tctl/common/plugins_command_test.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command_test.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command_test.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
Comment on lines 1231 to 1256
const (
// HostedPluginLabel defines the name for the hosted plugin label.
// When this label is set to "true" on a Plugin resource,
// it indicates that the Plugin should be run by the Cloud service,
// rather than self-hosted plugin services.
HostedPluginLabel = TeleportNamespace + "/hosted-plugin"
)

const (
// OktaOrgURLLabel is the label used by Okta-managed resources to indicate
// the upstream Okta organization that they come from.
OktaOrgURLLabel = "okta/org"

// OktaCredPurposeLabel is used by Okta-managed PluginStaticCredentials to
// indicate their purpose
OktaCredPurposeLabel = "okta/purpose"

// OktaCredPurposeAuth indicates that the credential is intended for
// authenticating with the Okta REST API
OktaCredPurposeAuth = "okta-auth"

// OktaCredPurposeSCIMToken indicates that theis to be used for authenticating
// SCIM requests from the upstream organization. The content of the credential
// is a bcrypt hash of actual token.
OktaCredPurposeSCIMToken = "scim-bearer-token"
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these constants are defined in teleport.e. Is there a corresponding teleport.e PR that removes them from there and uses these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, but its on the way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gravitational/teleport.e#4098

Required().
StringVar(&p.install.okta.apiToken)
p.install.okta.cmd.
Flag("saml-connector", "SAML connector to link to plugin").
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to "link" connector? Will it try to create connector with this name, or somehow adopt an existing one? It's not clear from this description.

tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
},
}

if _, err := args.plugins.CreatePlugin(ctx, req); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web UI doesn't let you create more than 1 instance of a hosted plugin, incl. Okta. How will this behave if you already have an Okta integration running?

And similarly, our UI prompts user to cleanup old Okta integration before creating a new one if it detects there are resources left from the previous instance of Okta integration. Does this behave the same way? @smallinsky should have details about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes It should behave the same way and the cleanup step will be retuned in error message.

Co-authored-by: Roman Tkachenko <[email protected]>
Co-authored-by: Jakub Nyckowski <[email protected]>
@marcoandredinis marcoandredinis removed their request for review May 2, 2024 10:22
Copy link
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let few minor comments.

Comment on lines 205 to 207
if oktaSettings.samlConnector == "" {
return trace.BadParameter("SCIM support requires saml-connector to be set")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why samlConnector is only validated when SCIM is enabled ?

Don't we reference to samlConnector in our okta sync flow ? For instance:
https://github.com/gravitational/teleport.e/blob/5012ca517540f8d1d44bda375fc5de0e8113e04a/lib/okta/synchronize.go#L640

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will just make it flat-out required

}
}

if oktaSettings.samlConnector != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if samlConnector was provided can we get the okta org from connector spec ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try it, and give a warning to set it explicitly if it can't be found. The App ID label is only guaranteed to be set if the SAML Connector was created though the Okta install flow; if it was created manually it may not exist.

tool/tctl/common/plugins_command.go Outdated Show resolved Hide resolved
func (p *PluginsCommand) initInstall(parent *kingpin.CmdClause, config *servicecfg.Config) {
p.install.cmd = parent.Command("install", "Install new plugin instance")

p.install.okta.cmd = p.install.cmd.Command("okta", "Install an okta integration")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
p.install.okta.cmd = p.install.cmd.Command("okta", "Install an okta integration")
p.install.okta.cmd = p.install.cmd.Command("okta", "Install an Okta integration")

Comment on lines 205 to 206
fmt.Printf("Failed validating SAML connector: %v", err)
return trace.Wrap(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fmt.Printf("Failed validating SAML connector: %v", err)
return trace.Wrap(err)
return trace.Wrap(err, "failed validating SAML connector")

Comment on lines 296 to 297
fmt.Printf("Plugin creation failed: %v\n", err)
return trace.Wrap(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fmt.Printf("Plugin creation failed: %v\n", err)
return trace.Wrap(err)
return trace.Wrap(err, "plugin creation failed")

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tcsc Can you include the docs for this as well?

@tcsc tcsc enabled auto-merge May 14, 2024 23:13
@tcsc tcsc added this pull request to the merge queue May 17, 2024
Merged via the queue into master with commit dfc127d May 17, 2024
38 checks passed
@tcsc tcsc deleted the tcsc/tctl-create-plugin branch May 17, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/md tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants